Introduce a #[diagnostic::on_unknown_item] attribute#152901
Introduce a #[diagnostic::on_unknown_item] attribute#152901weiznich wants to merge 1 commit intorust-lang:mainfrom
#[diagnostic::on_unknown_item] attribute#152901Conversation
|
Some changes occurred in compiler/rustc_passes/src/check_attr.rs |
rustbot
added
A-attributes
|
r? @nnethercote rustbot has assigned @nnethercote. Use Why was this reviewer chosen?The reviewer was selected based on:
|
This comment has been minimized.
This comment has been minimized.
weiznich
force-pushed
the
feature/on_unknown_item
branch
from
5c63f6f to
a9dd689
Compare
This comment has been minimized.
This comment has been minimized.
This PR introduces a `#[diagnostic::on_unknown_item]` attribute that allows crate authors to customize the error messages emitted by unresolved imports. The main usecase for this is using this attribute as part of a proc macro that expects a certain external module structure to exist or certain dependencies to be there. For me personally the motivating use-case are several derives in diesel, that expect to refer to a `tabe` module. That is done either implicitly (via the name of the type with the derive) or explicitly by the user. This attribute would allow us to improve the error message in both cases: * For the implicit case we could explicity call out our assumptions (turning the name into lower case, adding an `s` in the end) + point to the explicit variant as alternative * For the explicit variant we would add additional notes to tell the user why this is happening and what they should look for to fix the problem (be more explicit about certain diesel specific assumptions of the module structure) I assume that similar use-cases exist for other proc-macros as well, therefore I decided to put in the work implementing this new attribute. I would also assume that this is likely not useful for std-lib internal usage.
weiznich
force-pushed
the
feature/on_unknown_item
branch
from
a9dd689 to
c87fc9e
Compare
|
I can try this, although I need to read up on how the new infrastructure works and check if it's possible to use this inside of the name resolution stage. Somehow certain things (like lints) act weirdly there. |
|
The attribute refactor is pretty much finished, which means all old style parsers at this point have been removed from the compiler. There are many examples of new-parsing-infrastructure attribute parsers that work at this stage of the compiler (and none more that work using the old system). I don't think I want to accept any new old-style attribute parsers into the compiler anymore for that reason. r? me |
|
@jdonszelmann I can totally understand that you don't want to accept any attributes using the old style. Your comment as currently written is still not useful for me as person that contributes only from time to time to the compiler and doesn't keep up with all the internal changes all the time. I get that I need to change something, but it is really unclear for me:
It's especially not helpful to write that "There are many examples of new-parsing-infrastructure attribute parsers" without even providing a link to one of them. Do you have any documentation or other hints where to get these information from? Otherwise I fear it's impossible for me to satisfy these requests with the limited amount of time I'm able to spend on this change. |
|
Fair enough, take a look at how we handle |
|
☔ The latest upstream changes (presumably #153047) made this pull request unmergeable. Please resolve the merge conflicts. |
There was a problem hiding this comment.
#151558 has merged now so you can rebase on that.
What need to change exactly
- I think it'll look pretty similar to https://github.com/rust-lang/rust/blob/main/compiler/rustc_attr_parsing/src/attributes/diagnostic/on_const.rs
- Add a new variant to https://github.com/rust-lang/rust/blob/main/compiler/rustc_attr_parsing/src/attributes/diagnostic/mod.rs#L28
- I think we should not(?) support formatting parameters like
#[diagnostic::on_unknown_item(message = "{A}{B}{C}"]. To do that either change the logic in https://github.com/rust-lang/rust/blob/main/compiler/rustc_attr_parsing/src/attributes/diagnostic/mod.rs#L287 or fire lints like at https://github.com/rust-lang/rust/blob/main/compiler/rustc_passes/src/check_attr.rs#L630 . I think the former is easier, especially if you want to reject"{Self}"
I have some review comments as well. Thanks for continuing this work by the way :)
| const DIAG_ATTRS: &[Symbol] = | ||
| &[sym::on_unimplemented, sym::do_not_recommend, sym::on_const]; | ||
| &[sym::on_unimplemented, sym::do_not_recommend, sym::on_const, sym::on_unknown_item]; | ||
|
|
||
| if res == Res::NonMacroAttr(NonMacroAttrKind::Tool) | ||
| && let [namespace, attribute, ..] = &*path.segments | ||
| && namespace.ident.name == sym::diagnostic | ||
| && !DIAG_ATTRS.contains(&attribute.ident.name) | ||
| && (!DIAG_ATTRS.contains(&attribute.ident.name) | ||
| || (attribute.ident.name == sym::on_unknown_item | ||
| && !self.tcx.features().diagnostic_on_unknown_item())) |
There was a problem hiding this comment.
It would be nice to handle this in a more general way since on_const and (soon) on_move are also unstable. Such an expression is going to be unwieldy if we do this for those as well.
something like let diag_attrs = /* vec of all accessible diagnostic attrs */
| #![feature(diagnostic_on_unknown_item)] | ||
| pub mod foo { | ||
| pub struct Bar; | ||
| } | ||
|
|
||
| #[diagnostic::on_unknown_item( | ||
| message = "first message", | ||
| label = "first label", | ||
| note = "custom note", | ||
| note = "custom note 2" | ||
| )] | ||
| use foo::Foo; | ||
| //~^ERROR first message | ||
|
|
||
| use foo::Bar; | ||
|
|
||
| fn main() {} |
There was a problem hiding this comment.
Do you intend to support extern crate declarations? If so please add a test like
#[diagnostic::on_unknown_item(
message = "first message",
label = "first label",
note = "custom note",
note = "custom note 2"
)]
extern crate foo;Or if you don't, add it to incorrect-locations.rs
| //~^ ERROR: custom message | ||
| }; | ||
| } | ||
| fn main() {} |
There was a problem hiding this comment.
Please add a test for use inside the use declaration, like
mod test5 {
use std::{
string::String,
#[diagnostic::on_unknown_item(
message = "custom message",
label = "custom label",
note = "custom note"
)]
vec::{NonExisting, Vec},
//~^ ERROR: custom message
};
}6 participants
This PR introduces a
#[diagnostic::on_unknown_item]attribute that allows crate authors to customize the error messages emitted by unresolved imports. The main usecase for this is using this attribute as part of a proc macro that expects a certain external module structure to exist or certain dependencies to be there.For me personally the motivating use-case are several derives in diesel, that expect to refer to a
tabemodule. That is done either implicitly (via the name of the type with the derive) or explicitly by the user. This attribute would allow us to improve the error message in both cases:sin the end)I assume that similar use-cases exist for other proc-macros as well, therefore I decided to put in the work implementing this new attribute. I would also assume that this is likely not useful for std-lib internal usage.
related #152900 and #128674